Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add ReactScreen #170

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Add ReactScreen #170

wants to merge 6 commits into from

Conversation

jdpigeon
Copy link
Contributor

@jdpigeon jdpigeon commented May 5, 2022

This PR introduces a new type of screen that can be used to integrate React components into lab.js.

Usage is very simple

const Hello: React.FC<{ toWhat: string }> = (props) => {
  return <div>Hello {props.toWhat}</div>;
};

const experiment = new lab.flow.Sequence({
  content: [
    new ReactScreen({
      component: Hello,
      props: { toWhat: "World" } 
    })
  ]
})

jdpigeon
jdpigeon commented May 6, 2022
View reviewed changes
packages/library/src/react/index.ts Outdated
onRun() {
super.onRun()
this.componentDiv = document.createElement('div')
this.options.el?.appendChild(this.componentDiv)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This usage of the el option actually seems to have broken with the typescript update. I'm not seeing any active el attached to components now, even at run time.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this was part of the removal of runtime data from options. The component el is now part of the context in which a component is presented, which in turn is added while the study is running. I think you should be able to access the specific component via this.internals.context.el, cf. for example the canvas.Screen code

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I ended up doing is using global.rootEl. Is there any meaningful difference between that and context.el?

Another thing I did to make this nicer in the implementation of this in our library is add rootEl to the global interface in our equivalent of html.Screen

@FelixHenninger
Copy link
Owner

Hej Dano, thanks, this is fantastic! I think this is a great addition to the library, and something we should add for sure -- I don't see any blockers here.

I'm worried about a slightly auxiliary issue, and that's bundling react with the library -- with the PR, both the distribution and development version roughly double in size, which feels a lot to me. I wonder whether we can somehow provide react as an optional external dependency, rather than bundling it by default? My hunch is that larger applications using react are going to bundle the library themselves, so I'm not sure we need to ship it in the default bundle.
So, I'm wondering whether it would be possible to add react as an optional dependency? As an alternative, we could also add a (third) library flavor that includes the react screen and the react library.

What do you think? Thanks a lot for all of your efforts, I truly appreciate it! Again, this is something we should absolutely include (as we have discussed before), I'd love to figure out the bundling.

Kind regards, and many thanks, -Felix

@jdpigeon
Copy link
Contributor Author

Hej Dano, thanks, this is fantastic! I think this is a great addition to the library, and something we should add for sure -- I don't see any blockers here.

I'm worried about a slightly auxiliary issue, and that's bundling react with the library -- with the PR, both the distribution and development version roughly double in size, which feels a lot to me. I wonder whether we can somehow provide react as an optional external dependency, rather than bundling it by default? My hunch is that larger applications using react are going to bundle the library themselves, so I'm not sure we need to ship it in the default bundle. So, I'm wondering whether it would be possible to add react as an optional dependency? As an alternative, we could also add a (third) library flavor that includes the react screen and the react library.

What do you think? Thanks a lot for all of your efforts, I truly appreciate it! Again, this is something we should absolutely include (as we have discussed before), I'd love to figure out the bundling.

Kind regards, and many thanks, -Felix

Thanks for looking into the bundle issue a bit more. I agree that increasing the bundle size that much is not great. Let's continue with the original plan of creating a new lab.js-react package for this screen

@jdpigeon
Copy link
Contributor Author

I also found it necessary in our library to provide another option, containerStyle, for allowing the parent div to be styled. Will include it.

@FelixHenninger
Copy link
Owner

Let's discuss the status on this -- IIRC we discussed moving it into a separate package, but maybe I'm mistaken. Otherwise I just rebased my local branch and things are looking great, would love to have this in main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FelixHenninger FelixHenninger FelixHenninger left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jdpigeon @FelixHenninger